Repository navigation
Metal: define undefined phi values - #1083
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1083 +/- ##
==========================================
- Coverage 92.86% 91.93% -0.93%
==========================================
Files 31 31
Lines 7775 7787 +12
==========================================
- Hits 7220 7159 -61
- Misses 555 628 +73 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
The AGX back-end of M3 and newer GPUs miscompiles some phis that have an undef incoming value, even when that edge is never taken at run time. KernelAbstractions kernels routinely contain such phis: a variable assigned before a `@synchronize` and used after it is undefined on the path that skips the out-of-range threads. KernelAbstractions' `forloop` test then computed with the same local index in every thread, so all threads updated the same element, and similar kernels threw spurious bounds errors. Replace undef and poison phi operands with zero, after the loop-exit unification that introduces more of them.
maleadt
force-pushed
the
tb/metal-phi-undef
branch
from
November 7, 2026 16:11
2a5eb57 to
cd738f9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
On M3 and newer GPUs (Apple9+), KernelAbstractions'
Privatetestsuite fails: theforloopkernel leavesA[:, 1]unchanged. This reproduces on an M3 Pro (Metal.jl 1.12.0, GPUCompiler 2.21.0, KernelAbstractions 0.10.5) and was also reported on an M6. It passes on the M1 in Metal.jl's CI.The cause is a phi with an
undefincoming value. KernelAbstractions splits a kernel at each@synchronizeand wraps every section in a check that skips out-of-range threads. A variable assigned in one section and used in the next, likei = @index(Local, Linear)here, therefore becomesphi [%tid, %valid], [undef, %skipped]. Theundefedge is never taken by a thread that uses the value. The AIR handed to Metal still has this shape (checked by disassembling the.air), and it is valid IR. But the AGX back-end then has every thread compute with the same index. Each thread updates the same element per iteration, so after the loop every element has been incremented once instead of N times. Similar kernels throw a spuriousBoundsError.On the reduced kernel, changing only that
undefto any defined value (0, or the thread index) fixes the result. The barrier flags, bounds checks, exception lowering and@privateall turned out not to matter: the kernel still fails with all of them removed.This PR replaces
undefandpoisonphi operands with zero at the end oflower_air!. That is a valid refinement. It runs afterunify_loop_exits!, which introduces more such phis.Tested on an M3 Pro:
forloopkernel and 20+ reduced variants give correct results; the spuriousBoundsErroris gonemetaltests pass (new IR-level test included)